[ReachingDefAnalysis] Fix management of MBBFrameObjsReachingDefs #124943
Conversation
|
@mgudim I found that |
michaelmaitland
force-pushed
the
cleanup-rda-stack
branch
from
4ef33af to
18aa1a1
Compare
michaelmaitland
changed the title
|
Since there is nothing using this in llvm right now these changes shouldn't break anything. What do you think @michaelmaitland ? |
|
I had some trouble writing a test for the lookup part, since we were sort of getting lucky. What I mean by that is WRT to the Frame2InstrIdx bug, I would be surprised if that was NFC. I will try and write a test for it. |
|
Here is the memory usage compared to before your patch went in: @nikic is this reasonable? |
It's probably okay, max-rss is pretty noisy. |
michaelmaitland
force-pushed
the
cleanup-rda-stack
branch
from
45e9a32 to
b17fde3
Compare
|
I've added a test case. I pre-committed it in this patch so you can see the diff. |
michaelmaitland
force-pushed
the
cleanup-rda-stack
branch
from
b17fde3 to
4539a03
Compare
michaelmaitland
force-pushed
the
cleanup-rda-stack
branch
from
4539a03 to
af0058b
Compare
| {FrameIndex - ObjectIndexBegin, {CurInstr}}}; | ||
| } | ||
|
|
||
| int Key = FrameIndex - ObjectIndexBegin; |
There was a problem hiding this comment.
Why do we need to normalize the FrameIndex. Can't we use it as the key directly?
| } | ||
|
|
||
| int Key = FrameIndex - ObjectIndexBegin; | ||
| MBBFrameObjsReachingDefs[MBBNumber][Key].push_back(CurInstr); |
There was a problem hiding this comment.
A map of maps is a kind of silly data structure. You can make the key a std::pair and use a single level map. Unless we need to be able to easily access the inner map for a single basic block.
| auto &InnerMap = Lookup1->second; | ||
| auto Lookup2 = InnerMap.find(Key); | ||
| if (Lookup2 == InnerMap.end()) | ||
| auto Lookup = MBBFrameObjsReachingDefs.find({MBBNumber, Key}); |
There was a problem hiding this comment.
Doesn't this need to use FrameIndex instead of Key?
There was a problem hiding this comment.
yes, forgot to update. thanks
There was a problem hiding this comment.
So there's no testing? Or is it because object begin was 0?
There was a problem hiding this comment.
There is no testing for a non-zero ObjectIndexBegin, so it ended up behaving as if Key == FrameIndex. Do you want me to add a test for a non-zero ObjectIndexBegin?
There was a problem hiding this comment.
@topperc test added. It doesn't matter anymore though since we no longer care about ObjectIndexBegin.
…m#124943) MBBFrameObjsReachingDefs was not being built correctly since we were not inserting into a reference of Frame2InstrIdx. If there was multiple stack slot defs in the same basic block, then the bug would occur. This PR fixes this problem while simplifying the insertion logic. Additionally, when lookup into MBBFrameObjsReachingDefs was occurring, there was a chance that there was no entry in the map, in the case that there was no reaching def. This was causing us to return a default value, which may or may not have been correct. This patch returns the correct value now.
4 participants
MBBFrameObjsReachingDefs was not being built correctly since we were not inserting into a reference of Frame2InstrIdx. If there was multiple stack slot defs in the same basic block, then the bug would occur. This PR fixes this problem while simplifying the insertion logic.
Additionally, when lookup into MBBFrameObjsReachingDefs was occurring, there was a chance that there was no entry in the map, in the case that there was no reaching def. This was causing us to return a default value, which may or may not have been correct. This patch returns the correct value now.